fix(cli): wire praison run session continuity to project session store#1963
fix(cli): wire praison run session continuity to project session store#1963cursor[bot] wants to merge 2 commits into
Conversation
The CLI session continuity feature (#1929) discovered sessions in the project-scoped store but agents saved/loaded from the global default store. auto_save alone did not set session_id or enable history, so prior messages were never restored and new messages were silently lost. - Add build_cli_memory_config() and apply_cli_session_continuity() - Wire project store in handle_direct_prompt and praison run actions mode - Preserve session args across praison.run() arg re-parse for YAML runs - Respect caller-injected session store in _init_session_store() - Fix --output actions crash from invalid resume_session Agent kwarg Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
✅ Action performedReview finished.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughTwo new helper functions— ChangesCLI Project-Scoped Session Continuity
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as run.py / main.py
participant project_sessions as project_sessions.py
participant ProjectSessionStore
participant Agent as PraisonAgent
User->>CLI: praisonai run --resume / --auto-save
CLI->>project_sessions: build_cli_memory_config(session_id, auto_save)
project_sessions-->>CLI: MemoryConfig(history=True, save_name=...)
CLI->>Agent: PraisonAgent(**agent_config with memory)
CLI->>project_sessions: apply_cli_session_continuity(agent, session_id)
project_sessions->>ProjectSessionStore: get_project_session_store()
project_sessions->>Agent: set _session_store, _session_id, _save_history
project_sessions->>ProjectSessionStore: get_history(session_id)
ProjectSessionStore-->>project_sessions: prior messages
project_sessions->>Agent: agent.chat_history = prior messages
Agent-->>User: response (with restored context)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/praisonai/praisonai/cli/commands/run.py (1)
460-517:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftProfiled runs still bypass the new session-continuity flow.
Both profiled helpers accept
continue_session,session,fork, andno_save, but neither path uses them when constructingPraisonAI/Agent.praison run --profile --continue/--sessionwill therefore still behave differently from the normal path and miss the project-scoped restore/save wiring this PR is trying to standardize.Also applies to: 519-578
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai/praisonai/cli/commands/run.py` around lines 460 - 517, The `_run_from_file_profiled` function (and similarly the profiled helper at lines 519-578) accepts session-continuity parameters `continue_session`, `session`, `fork`, and `no_save` but does not pass them to the `PraisonAI` constructor. To fix this, pass these parameters when constructing the `praison` object alongside `agent_file` and `framework`, ensuring the profiled execution paths respect the same session continuity logic as the non-profiled paths. Apply the same fix to both profiled functions to maintain consistency across all execution modes.
🧹 Nitpick comments (3)
src/praisonai/tests/unit/cli/test_project_session_continuity.py (3)
1-58: 💤 Low valueConsider adding edge case coverage.
The current tests focus on happy paths. To improve robustness, consider adding tests for:
build_cli_memory_configwithNoneor emptysession_id/auto_saveapply_cli_session_continuitywith a nonexistent session (should it gracefully handle an empty history?)- Error scenarios like store I/O failures or invalid session data
This would increase confidence in error handling and boundary conditions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai/tests/unit/cli/test_project_session_continuity.py` around lines 1 - 58, Add new test functions to cover edge cases for the session continuity features. Create test functions for: (1) `build_cli_memory_config` behavior with None or empty values for session_id and auto_save parameters; (2) `apply_cli_session_continuity` behavior when called with a nonexistent session_id to ensure it gracefully handles empty chat history; (3) error scenarios such as when the session store encounters I/O failures or receives invalid session data. Each test should verify that the functions either handle the edge case gracefully (returning expected defaults or empty states) or raise appropriate exceptions with clear error messages.
52-52: 💤 Low valueConsider reducing coupling to private implementation details.
Similar to the previous test, this accesses
agent._session_store.session_dirand directly callsagent._init_session_store(). While this validates the internal lazy-init contract (per the PR fix), it couples the test to private methods. If the init mechanism changes, the test may break unnecessarily.Document that this test intentionally validates the internal session store injection contract, or expose a public inspection API if broader testing needs exist.
Also applies to: 54-55
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai/tests/unit/cli/test_project_session_continuity.py` at line 52, The test accesses private implementation details like agent._session_store.session_dir and agent._init_session_store() which creates unnecessary coupling. Add a clear comment above the line accessing injected_dir (line 52) and the subsequent lines (54-55) that documents the test's intent: that it is intentionally validating the internal lazy-initialization contract of the session store, explaining why direct access to these private members is necessary for this specific validation. This makes the tight coupling explicit and justified, clarifying to future maintainers that this is not casual private API usage but rather a targeted contract test.
29-30: 💤 Low valueConsider reducing coupling to private implementation details.
The test accesses private attributes (
agent._session_store,agent._session_id) and calls a private method (agent._auto_save_session()). While acceptable for unit tests, this creates tight coupling to internal structure. If the Agent implementation changes (e.g., renames_session_idor changes auto-save mechanics), this test will break even if the public contract remains stable.Consider whether public APIs exist for these checks (e.g., a
get_session_id()method or asave()method), or document that this test intentionally validates internal wiring as part of the integration contract.Also applies to: 36-36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai/tests/unit/cli/test_project_session_continuity.py` around lines 29 - 30, The test is tightly coupled to private implementation details like agent._session_store, agent._session_id, and agent._auto_save_session(), which will break if the internal implementation changes. Review whether public APIs exist (such as a get_session_id() method or a save() method) that can be used instead of accessing private attributes and methods. If public APIs exist, refactor the assertions to use them. If no public APIs exist and these are essential to validate, add a clear comment documenting that this test intentionally validates internal wiring as part of the integration contract, acknowledging the tight coupling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/praisonai/praisonai/cli/commands/run.py`:
- Line 259: The cli_project_sessions assignment only checks auto_save_name, but
fails to account for session resumption via the --session/--continue flag. When
these options are combined with --no-save, session_id is still set but
auto_save_name becomes None, causing cli_project_sessions to incorrectly become
False and skip downstream project-store wiring for read-only resumes. Modify the
assignment of cli_project_sessions to include a check for the resume signal (the
session continuation flag) in addition to the auto_save_name check, so that
project sessions remain active when resuming even if saving is disabled. Apply
this same logic fix at all locations where cli_project_sessions is assigned
based on auto_save_name.
In `@src/praisonai/praisonai/cli/main.py`:
- Around line 4821-4825: The new session-continuity hook in
handle_direct_prompt() at the getattr(self.args, 'cli_project_sessions', False)
line unconditionally dereferences self.args without checking if it exists first.
Since the rest of handle_direct_prompt() uses hasattr(self, "args") guards to
protect against programmatic callers that invoke the method before parse_args()
completes, add the same guard pattern before the entire if block that checks for
cli_project_sessions and calls apply_cli_session_continuity. Wrap the session
continuity logic (lines checking cli_project_sessions and calling
apply_cli_session_continuity) inside a hasattr(self, "args") guard to prevent
AttributeError when self.args is unset.
In `@src/praisonai/praisonai/cli/state/project_sessions.py`:
- Around line 114-116: The build_cli_memory_config() function is incorrectly
re-enabling auto-save by using session_id as a fallback when auto_save is not
provided. The line save_name = auto_save or sid causes new turns to be persisted
even when the user explicitly requested no persistence with --no-save. Fix this
by only setting auto_save in the MemoryConfig when it was explicitly provided by
the user, not by falling back to the session_id. The same issue also appears in
apply_cli_session_continuity() at lines 126-127 where the agent configuration is
being set, so apply the same fix there to prevent re-enabling persistence in
resume-only runs.
---
Outside diff comments:
In `@src/praisonai/praisonai/cli/commands/run.py`:
- Around line 460-517: The `_run_from_file_profiled` function (and similarly the
profiled helper at lines 519-578) accepts session-continuity parameters
`continue_session`, `session`, `fork`, and `no_save` but does not pass them to
the `PraisonAI` constructor. To fix this, pass these parameters when
constructing the `praison` object alongside `agent_file` and `framework`,
ensuring the profiled execution paths respect the same session continuity logic
as the non-profiled paths. Apply the same fix to both profiled functions to
maintain consistency across all execution modes.
---
Nitpick comments:
In `@src/praisonai/tests/unit/cli/test_project_session_continuity.py`:
- Around line 1-58: Add new test functions to cover edge cases for the session
continuity features. Create test functions for: (1) `build_cli_memory_config`
behavior with None or empty values for session_id and auto_save parameters; (2)
`apply_cli_session_continuity` behavior when called with a nonexistent
session_id to ensure it gracefully handles empty chat history; (3) error
scenarios such as when the session store encounters I/O failures or receives
invalid session data. Each test should verify that the functions either handle
the edge case gracefully (returning expected defaults or empty states) or raise
appropriate exceptions with clear error messages.
- Line 52: The test accesses private implementation details like
agent._session_store.session_dir and agent._init_session_store() which creates
unnecessary coupling. Add a clear comment above the line accessing injected_dir
(line 52) and the subsequent lines (54-55) that documents the test's intent:
that it is intentionally validating the internal lazy-initialization contract of
the session store, explaining why direct access to these private members is
necessary for this specific validation. This makes the tight coupling explicit
and justified, clarifying to future maintainers that this is not casual private
API usage but rather a targeted contract test.
- Around line 29-30: The test is tightly coupled to private implementation
details like agent._session_store, agent._session_id, and
agent._auto_save_session(), which will break if the internal implementation
changes. Review whether public APIs exist (such as a get_session_id() method or
a save() method) that can be used instead of accessing private attributes and
methods. If public APIs exist, refactor the assertions to use them. If no public
APIs exist and these are essential to validate, add a clear comment documenting
that this test intentionally validates internal wiring as part of the
integration contract, acknowledging the tight coupling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d8466d4-29af-41ca-a748-ae91c58ace31
📒 Files selected for processing (5)
src/praisonai-agents/praisonaiagents/agent/memory_mixin.pysrc/praisonai/praisonai/cli/commands/run.pysrc/praisonai/praisonai/cli/main.pysrc/praisonai/praisonai/cli/state/project_sessions.pysrc/praisonai/tests/unit/cli/test_project_session_continuity.py
| args = Args() | ||
| args.auto_save = auto_save_name | ||
| args.resume_session = session_id | ||
| args.cli_project_sessions = bool(auto_save_name) |
There was a problem hiding this comment.
Use the resume signal when deciding whether project sessions are active.
These assignments only look at auto_save_name. When --session/--continue is combined with --no-save, session_id is still set but auto_save_name is None, so cli_project_sessions flips to False and the downstream main.py project-store wiring is skipped for read-only resumes.
Suggested fix
- args.cli_project_sessions = bool(auto_save_name)
+ args.cli_project_sessions = bool(session_id or auto_save_name)- args.cli_project_sessions = bool(auto_save_name)
+ args.cli_project_sessions = bool(session_id or auto_save_name)Also applies to: 416-416
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai/praisonai/cli/commands/run.py` at line 259, The
cli_project_sessions assignment only checks auto_save_name, but fails to account
for session resumption via the --session/--continue flag. When these options are
combined with --no-save, session_id is still set but auto_save_name becomes
None, causing cli_project_sessions to incorrectly become False and skip
downstream project-store wiring for read-only resumes. Modify the assignment of
cli_project_sessions to include a check for the resume signal (the session
continuation flag) in addition to the auto_save_name check, so that project
sessions remain active when resuming even if saving is disabled. Apply this same
logic fix at all locations where cli_project_sessions is assigned based on
auto_save_name.
| if getattr(self.args, 'cli_project_sessions', False): | ||
| session_id = getattr(self.args, 'resume_session', None) or getattr(self.args, 'auto_save', None) | ||
| if session_id: | ||
| from .state.project_sessions import apply_cli_session_continuity | ||
| apply_cli_session_continuity(agent, session_id) |
There was a problem hiding this comment.
Guard the new session-continuity hook when self.args is unset.
Line 4821 now dereferences self.args unconditionally inside handle_direct_prompt(). This method otherwise uses hasattr(self, "args") guards throughout, so programmatic callers that invoke it before main()/parse_args() will now crash with AttributeError right after agent construction.
Suggested fix
- if getattr(self.args, 'cli_project_sessions', False):
- session_id = getattr(self.args, 'resume_session', None) or getattr(self.args, 'auto_save', None)
+ if hasattr(self, 'args') and getattr(self.args, 'cli_project_sessions', False):
+ session_id = getattr(self.args, 'resume_session', None) or getattr(self.args, 'auto_save', None)
if session_id:
from .state.project_sessions import apply_cli_session_continuity
apply_cli_session_continuity(agent, session_id)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai/praisonai/cli/main.py` around lines 4821 - 4825, The new
session-continuity hook in handle_direct_prompt() at the getattr(self.args,
'cli_project_sessions', False) line unconditionally dereferences self.args
without checking if it exists first. Since the rest of handle_direct_prompt()
uses hasattr(self, "args") guards to protect against programmatic callers that
invoke the method before parse_args() completes, add the same guard pattern
before the entire if block that checks for cli_project_sessions and calls
apply_cli_session_continuity. Wrap the session continuity logic (lines checking
cli_project_sessions and calling apply_cli_session_continuity) inside a
hasattr(self, "args") guard to prevent AttributeError when self.args is unset.
| sid = session_id or auto_save | ||
| save_name = auto_save or sid | ||
| return MemoryConfig(session_id=sid, auto_save=save_name, history=True) |
There was a problem hiding this comment.
Don't re-enable auto-save on resume-only runs.
build_cli_memory_config() currently turns session_id back into auto_save, and apply_cli_session_continuity() repeats that fallback on the agent. With --session/--continue --no-save, this will still write new turns into the session the user explicitly asked not to persist.
Suggested direction
def build_cli_memory_config(
session_id: Optional[str] = None,
auto_save: Optional[str] = None,
):
@@
from praisonaiagents import MemoryConfig
sid = session_id or auto_save
- save_name = auto_save or sid
- return MemoryConfig(session_id=sid, auto_save=save_name, history=True)
+ return MemoryConfig(session_id=sid, auto_save=auto_save, history=True)
-def apply_cli_session_continuity(agent, session_id: str, project_path: Optional[str] = None) -> None:
+def apply_cli_session_continuity(
+ agent,
+ session_id: str,
+ project_path: Optional[str] = None,
+ auto_save: Optional[str] = None,
+) -> None:
@@
- if not getattr(agent, "auto_save", None):
- agent.auto_save = session_id
+ if auto_save is not None:
+ agent.auto_save = auto_saveAlso applies to: 126-127
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai/praisonai/cli/state/project_sessions.py` around lines 114 -
116, The build_cli_memory_config() function is incorrectly re-enabling auto-save
by using session_id as a fallback when auto_save is not provided. The line
save_name = auto_save or sid causes new turns to be persisted even when the user
explicitly requested no persistence with --no-save. Fix this by only setting
auto_save in the MemoryConfig when it was explicitly provided by the user, not
by falling back to the session_id. The same issue also appears in
apply_cli_session_continuity() at lines 126-127 where the agent configuration is
being set, so apply the same fix there to prevent re-enabling persistence in
resume-only runs.
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Architecture Review & Bug Fix ✅Conducted final architecture review and successfully fixed all valid issues found by CodeRabbit, Qodo, and other reviewers. Todo List:
Architecture Compliance ✅The changes follow the protocol-driven design correctly:
Fixed Issues:✅ Issue 1: Fixed ✅ Issue 2: Added ✅ Issue 3: Removed automatic auto-save fallback in ✅ Issue 4: Added session continuity support to both profiled execution paths - ensures consistent behavior across all CLI modes Changes Made:
Final Verdict: ✅ APPROVEDAll architectural and functional issues identified in the review have been resolved. The PR now:
The PR is ready for merge. 🔗 Branch: |
- Fix cli_project_sessions logic to check both session_id and auto_save_name - Add hasattr(self, 'args') guard in main.py session continuity hook - Remove auto-save re-enabling fallback in project_sessions.py - Add session continuity support to profiled execution paths Fixes issues identified in PR review to ensure --no-save and --continue work correctly together without silently re-enabling persistence. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
2 participants
Bug and impact
praison run --continue/--session(#1929) discovered sessions in the project-scoped store (~/.praisonai/sessions/projects/...) but agents never loaded or saved there.auto_savealone did not setsession_idor enable history, so:--output actions --session <id>crashed withTypeErrorfrom invalidresume_sessionAgent kwargRoot cause
Session discovery used
ProjectSessionStore, but agent persistence usedget_default_session_store()with nosession_idwired.handle_direct_promptignoredargs.resume_session, andpraison.run()re-parsed CLI args, dropping session flags set by the run command.Fix
build_cli_memory_config()andapply_cli_session_continuity()to wire agents to the project storehandle_direct_prompt,praison runactions mode, and YAML cli_config extractionpraison.run()arg re-parse_session_storein_init_session_store()Validation
src/praisonai/tests/unit/cli/test_project_session_continuity.pySummary by CodeRabbit
New Features
Tests